-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigation: Remove the check for draft navigation menus from the UnsavedInnerBlocks component #49161
Conversation
Size Change: -31 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for persisting with this. It's important to reduce the requests.
I'm a bit nervous about deleting this code without a clear rationale behind why it would be safe to do so.
I left a suggestion...LMK.
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
461d4c6
to
623af6a
Compare
@scruffian I looked at this again and saw this line:
I assume we need all menus so when the menu name is created it isn't a duplicate? This would only be a factor however if the code in Looking I can see that Pinging @talldan as well because I feel like he might have written this component originally. |
Noting also that we have some e2e test coverage for this functionality now. I guess we could amend that to prepopulate with a draft and a published menu and then assert that when inner blocks change only a single menu gets created and that it's title doesn't clash with the other ones. |
I think it's just because it has been refactored, and some dead code wasn't removed. In the past the menu creation/naming code was inside |
I wondering if we could change the resolution checker to check for both publish and draft? That way if the code in the |
The problem with the approach in this PR is that adding the page list as fallback ends up loading the unsaved inner blocks component which then ends up creating a wp_navigation menu. I think we should wait for #48698 to land and then reasses what we should do here... |
I don't understand why that would happen. It should (and did!) happen like this:
|
4627173
to
277b515
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked as described.
Also have a dedicated selector for the resolution status of all menus is a lot more resilient to change.
Thank you!
What?
I was trying to track this down in #49143 but I was looking in the wrong place! This removes a check for loaded draft navigation menus from the
UnsavedInnerBlocks
component.Why?
I believe it is unnecessary to make this request, since we are already fetching both draft and published navigations in the
useNavigationMenu
hook.How?
Deleting code!
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Network tab with only one GET: